fix(shape): Fix errors related to multi-value shape categories #4547
Conversation
|
Looks like I missed replacing one instance of an old variable name. That should be fixed now. |
|
All 633 screenshot tests passed for commit 95ede35 vs. |
|
After merging from master after #4548 was merged, I encountered new errors. I've now resolved those, but it involved some surgery to the order of operations performed in the mdc-shape functions. The end result is that mdc-shape now better supports categories having multi-radius values (whereas our current code was clearly assuming they'd be only a single radius value). |
| @@ -70,39 +72,10 @@ | |||
| } | |||
| } | |||
|
|
|||
| @function mdc-shape-resolve-percentage-for-corner_($component-height, $radius) { | |||
There was a problem hiding this comment.
I moved this function lower in the file verbatim, to group internal functions together.
| // | ||
| // 50% => 50 | ||
| // | ||
| @function mdc-shape-strip-unit_($number) { |
There was a problem hiding this comment.
This function was apparently not being used whatsoever, so I removed it.
|
|
||
| @each $corner in $radius { | ||
| $radius-value: append($radius-value, mdc-shape-prop-corner-value_($corner)); |
There was a problem hiding this comment.
I ended up merging the logic of mdc-shape-prop-corner-value_ into this function after some of the finagling I did led to this function otherwise pretty much not doing anything important in itself besides validating list length.
| $corner: nth($radius, $i); | ||
|
|
||
| @if map-has-key($mdc-shape-category-values, $corner) { | ||
| // If a category is encountered within a list of radii, apply the category's value for the corresponding corner |
There was a problem hiding this comment.
This behavior maximizes compatibility with the existing behavior which clearly expected shape categories to be applicable to individual corners (e.g. small small 0 0). This will effectively behave identically to the existing behavior in cases where a shape category is a single radius, while making list-of-radii category values actually functional.
| } | ||
| } | ||
|
|
||
| @function mdc-shape-validate-radius-value_($radius) { |
There was a problem hiding this comment.
This function has been rewritten to emit an error, akin to the verify functions in feature-targeting, so that there's less code to write in places that call this.
|
All 633 screenshot tests passed for commit 5904bd1 vs. |
kfranqueiro
changed the title
|
All 633 screenshot tests passed for commit dfe2347 vs. |
|
|
||
| @each $corner in $radius { | ||
| $radius-value: append($radius-value, mdc-shape-prop-corner-value_($corner)); | ||
| @for $i from 1 through length($radius) { |
There was a problem hiding this comment.
Curious, why is this changed from @each to @for?
There was a problem hiding this comment.
I need the index to correlate between the given radius list and the fully-expanded list of radii for any categories that are referenced.
| @for $i from 1 through length($radius) { | ||
| $corner: nth($radius, $i); | ||
|
|
||
| @if map-has-key($mdc-shape-category-values, $corner) { |
There was a problem hiding this comment.
What happens if small value is 4px 0 4px 0 and I apply this mixin with this value - mdc-button-shape-radius(small 0 small 0)?
I'm OK not supporting something like mdc-button-shape-radius(small 0 small 0) if that makes the implementation complex.
There was a problem hiding this comment.
That would result in 4px 0 4px 0. It maps each respective corner from the shape category. This also allows existing cases that expand to e.g. small small 0 0 to continue to work as-is, even if small is only a single value (it'll expand to the same value on all 4 corners).
|
All 633 screenshot tests passed for commit f76d287 vs. |
|
🤖 Beep boop! Screenshot test report 🚦3 screenshots changed from Details3 Changed: |
kfranqueiro
dismissed
moog16’s stale review
The build errors have been resolved.
(cherry picked from commit 9f79d17)
Refs #4140.
This PR changes the behavior from throwing an error and failing compilation, to warning about each encountered case and automatically falling back to using the first radius value. This is done in such a way as to avoid this problem even when multiple values are assigned at the shape variable level (i.e. centrally for small/medium/large).
I have a separate WIP branch to actually support multiple corner radii for notched-outline-related components (see the bug).
To test, assign multiple radii to the small/medium
$mdc-shape-...-component-radiusvariable(s) inmdc-shape/_variables.scss.